Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flip argument order of takeUntil module #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nordfjord
Copy link
Collaborator

The takeUntil module violates one of the first principles of functional
programming. Its argument order was (src, end) which is a data first
argument order.

This PR flips the argument order to (end, src) allowing us to better
use takeUntil in pipelines such as:

const query = stream('');

// Start fetching the results but if they haven't
// arrived when a new query is emitted
// stop listening to the stream
const results = query
 .chain(q =>
    fromPromise(getResults(q))
      .pipe(takeUntil(query))
  )

Which could also be written as

const query = stream('');

const getResults$ = chain(compose (takeUntil(query), fromPromise, getResults))

const results = getResults$(query);

If you prefer compose to dot-chains.

@nordfjord nordfjord force-pushed the feature/flip-takeUntil-arg-order branch 2 times, most recently from 913d659 to 46ef88c Compare April 25, 2018 18:05
@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 96c915c on nordfjord:feature/flip-takeUntil-arg-order into 0b1a435 on paldepind:master.

@nordfjord
Copy link
Collaborator Author

This would close #121

```

__Signature__

`Stream a -> Stream b -> Stream a`
`Stream b -> Stream a -> Stream a`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should flip the meaning of a and b instead of flipping their order? As in Stream a -> Stream b -> Stream b?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! yes of course! that makes much more sense, nice catch!

@paldepind
Copy link
Owner

Good observation. I agree. The new argument order is more natural for currying 👍

@nordfjord nordfjord force-pushed the feature/flip-takeUntil-arg-order branch 2 times, most recently from e490924 to a9e1471 Compare July 11, 2018 18:55
@nordfjord
Copy link
Collaborator Author

I think this would qualify as a breaking change, so I think we might need to bump the version to 0.3.x

@nordfjord nordfjord force-pushed the feature/flip-takeUntil-arg-order branch from a9e1471 to c7f2e16 Compare July 11, 2018 18:59
The takeUntil module violates one of the first principles of functional
programming. Its argument order was (src, end) which is a data first
argument order.

This commit flips the argument order to (end, src) allowing us to better
use takeUntil in pipelines such as:

```js
const query = stream('');

// Start fetching the results but if they haven't
// arrived when a new query is emitted
// stop listening to the stream
const results = query
 .chain(q =>
    fromPromise(getResults(q))
      .pipe(takeUntil(query))
  )
```

This also fixes an issue with takeUntil where if the new endStream had a
value the dependent stream wouldn't update.
@nordfjord nordfjord force-pushed the feature/flip-takeUntil-arg-order branch from c7f2e16 to 96c915c Compare July 12, 2018 14:59
var drop1 = flyd.transduce(drop(1));

module.exports = flyd.curryN(2, function(term, src) {
var end$ = flyd.merge(term.hasVal ? drop1(term) : term, src.end);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To maintain the behaviour of takeUntil as it was before #180 we need to drop a single value from the terminator stream if it already has a value, otherwise the stream will end immediately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an example of code that wouldn't work without it would be:

const s = stream();
const result$ = s.chain(q => fromPromise(getSearchResults(q)))
  .pipe(takeUntil(s))
  .map(results => h('ul', results.content.map(res => h('li', res))))
  .map(render('#results'));

In the above example you would only be able to search once. Any subsequent search would be ended immediately and not return a value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants